Escape angle brackets in attachment JSON so pasted attachments survive DOMPurify - #1338
Conversation
02743cc to
aafa512
Compare
There was a problem hiding this comment.
Pull request overview
Escapes angle brackets in attachment JSON before rendering and sanitization, preventing DOMPurify from dropping attachments during paste.
Changes:
- Added lossless JSON angle-bracket escaping.
- Applied escaping during attachment rendering and pre-sanitization.
- Added comprehensive unit and system regression coverage.
Tip
If you aren't ready for review, convert to a draft PR.
Click "Convert to draft" or run gh pr ready --undo.
Click "Ready for review" or run gh pr ready to reengage.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
src/trix/views/attachment_view.js |
Escapes emitted attachment JSON attributes. |
src/trix/models/html_sanitizer.js |
Escapes valid legacy JSON before DOMPurify. |
src/trix/core/helpers/strings.js |
Adds the escaping helper. |
src/test/unit/html_sanitizer_test.js |
Covers DOMPurify trigger sequences. |
src/test/unit/html_parser_test.js |
Tests attachment parsing during paste. |
src/test/unit/helpers/strings_test.js |
Tests escaping and round-trip behavior. |
src/test/unit/document_view_test.js |
Verifies safely rendered attributes. |
src/test/unit.js |
Registers the new helper tests. |
src/test/test_helpers/fixtures/fixtures.js |
Updates expected attachment markup. |
src/test/test_helpers/editor_helpers.js |
Adds stored attachment HTML generation. |
src/test/system/pasting_test.js |
Adds end-to-end paste regressions. |
action_text-trix/app/assets/javascripts/trix.js |
Updates the generated browser bundle. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
2515e30 to
affbe0f
Compare
affbe0f to
62eab4c
Compare
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 62eab4c646
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
c1e9df8 to
8dd9a1f
Compare
…e DOMPurify Copying an attachment out of Trix and pasting it back silently dropped it whenever the attachment JSON contained "</style>" or another sequence DOMPurify's SAFE_FOR_XML mode treats as a raw-text or comment terminator. Paste runs the clipboard's text/html through HTMLParser under SAFE_FOR_XML, and DOMPurify's attribute rule removes the whole data-trix-attachment attribute on a match, before the forceKeepAttr set by Trix's uponSanitizeAttribute hook is honored. Quoted mail carrying an embedded <style> block is the common case. Dragging the same content was lossless. Escape "<" and ">" inside the JSON as "\u003c" and "\u003e" at both ends: AttachmentView emits data-trix-attachment and data-trix-attributes that way, so Trix's own HTML never carries a trigger sequence, and HTMLSanitizer rewrites those attributes before DOMPurify sees them, so stored, server- rendered and older-Trix HTML with literal brackets survives too. In JSON text angle brackets only occur inside string literals, where the escapes spell the same characters, so JSON.parse reads back the same value; the sanitizer only rewrites values that already parse as JSON.
#1337 stashed every data-trix-* attribute DOMPurify's SAFE_FOR_XML pass dropped and restored it in afterSanitizeAttributes. The escaping added here makes that unnecessary: sanitizeElement escapes the angle brackets in the JSON attachment attributes before DOMPurify runs, so SAFE_FOR_XML never drops them and forceKeepAttr keeps them. The blanket restore's only remaining effect was re-admitting malformed or non-JSON data-trix-* values the pass deliberately dropped, so remove it and the module-global stash and rely on escaping. Non-JSON attachment attributes are unusable on read anyway.
8dd9a1f to
0004e08
Compare
|
🤖 Rebased onto the rebased #1337 branch (itself on main@47004013). GitHub now reports MERGEABLE. The CONFLICTING state was real, not a stale recompute: the base branch had been force-pushed to a rebased copy of its own commits after this branch was built on the older SHAs, so the same two patches existed twice at different SHAs. Replaying this branch's two commits onto the current base clears it. Review changes:
Tests (Node 18.20.8, Playwright Chromium):
Negative controls: removing the read-side escape in Fuzzed Interaction to settle before merging: #1334 also edits Worth a maintainer's eye: this changes the shape of the HTML Trix saves — This PR had no independent adversarial review round in this pass — the reviewer pool was saturated. Everything above is my own review plus the negative controls and fuzzing named. CI after this push: 20 of 21 checks green, including Browser tests (Sauce: Windows Chrome, Firefox and Edge, 476 passed each) and the full Action Text matrix. The one red check, "Downstream Rails integration tests", is not from this branch. I dispatched a control run of the same workflow on unmodified |
…ments Pasted HTML is trimmed at "</html>" because Windows browsers can append clipboard bytes after it, which the parser would otherwise append to the body as text. The trim ran as a plain string replace, so a "</html>" inside an attribute value cut the string mid-attribute and dropped everything from that element on. Attachment JSON hits this whenever the content is a full HTML document, as stored and server-rendered attachments carry it with literal angle brackets: pasting a quoted mail attachment lost the attachment and everything after it. Scan for the closing tag as a tag, skipping over quoted attribute values and comments, so only the real end tag ends the document.
|
Two follow-ups from a further adversarial pass, pushed as 6b945e8 and b09fc9f.
Non-JSON PR body updated to match (the restore-hook paragraph was stale since 0004e08). Bundle rebuilt and idempotent; 533 tests, 504 passed, 0 failed, 29 skipped. @codex review |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
🟡 Changes recommended
The closing-tag scanner can still truncate valid pasted content when </html> appears in raw-text elements or similarly named end tags.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
src/trix/models/html_sanitizer.js:168
- The prefix check also accepts unrelated end tags such as
</htmlish>. The browser ignores that unknown end tag, but this function truncates everything after it. Require a tag-name boundary before treating the token as the closinghtmltag.
if (/^<\/html/i.test(match[0])) {
- Files reviewed: 12/12 changed files
- Comments generated: 1
- Review effort level: Balanced
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b09fc9f131
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
The tag pattern let whitespace and an unquoted attribute value match the same characters more than one way, so a pasted string such as "<A !=" with " !=" repeated took exponential time to reject: 50 seconds for 125 characters. Give every part of a tag a single way to match, and let an unterminated comment, quoted value or tag run to the end of the string as the HTML tokenizer does, so a scan never restarts inside one.
A regex that knows about attribute values and comments still isn't the tokenizer: a "</html>" inside a style or textarea element, or a comment closed with "--!>", was taken for the closing tag, and each such case would need its own rule. Ask the browser instead. Swap every "</html" in the string for a marker start tag and parse once; the first marker that comes out as an element was tokenized as a tag, so that is where the document ends, and any marker inside an attribute value, a comment or raw text stays text. One extra parse, only when the string contains "</html" at all, and no String.prototype.matchAll for the Safari 12.1 target.
|
Pushed cedf67e on top of 4f98974, addressing all four threads at the layer they point at: the regex tokenizer is gone. Every @codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cedf67e4ed
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
The marker carried its offset in a quoted attribute value, and that quote changed the tokenizer's state when the marker landed inside a quoted attribute value: with two "</html>" in one value, the first marker closed the value and its element, and the second was parsed as a real tag. A quoted mail chain with two HTML documents in one attachment hit this. Write the offset unquoted, so the marker carries nothing that changes state in any context. Accept only markers whose offset points at a "</html" in the source, so an element of the same name supplied by the input can't stand in for one, and bound the tag name by HTML's ASCII whitespace rather than the JavaScript \s class, which the tokenizer doesn't share.
|
Pushed 2b079b2 for the three threads on cedf67e: the marker's offset is now an unquoted attribute value (no tokenizer-state characters), only markers whose offset points at a real @codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2b079b2382
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
An element in the input with the marker's name and an offset pointing at a "</html" inside an attribute value passed for a marker and cut the paste there. Give the marker a name chosen per call instead of checking offsets after the fact: nothing in the input can carry a name it doesn't know, so every element found is one the scan inserted.
|
Converged at Gate. CI run 34406221095 on this head is 20 of 21 green: Browser tests on Sauce (Windows 10 Chrome/Firefox/Edge: 506 passed, 0 failed, 30 skipped each; Android Chrome: 507 passed, 0 failed, 29 skipped; 536 tests), the full Action Text matrix (13/13), GitHub Actions audit, and CodeQL with its three Analyze jobs (the earlier inefficient-regex alert is gone with the regex). The one red, Downstream Rails integration tests, is red on trix Threads. 10 total, 10 fixed, 0 declined: the Rounds. Six reviewer rounds since opening: Copilot twice on 2026-08-28, Codex on 08-30, then on 09-09 CodeQL + Copilot + Codex (20:37 to 20:46Z), Codex at 21:05Z, Codex at 21:15Z, each re-summoned on the push it reviewed. Merge order stands: #1337, then this, then optionally #1353 (stacked on this branch); #1334 closes, its hook obviated by the escaping here. Nothing merged. |
Stacked on #1337 (base:
fix-data-trix-attributes-safe-for-xml). This diff is only the layer on top; merge #1337 first.The bug
Copying an attachment out of Trix and pasting it back silently dropped it whenever the attachment JSON contained
</style>or another sequence DOMPurify'sSAFE_FOR_XMLmode treats as a raw-text or comment terminator. Paste runs the clipboard'stext/htmlthroughHTMLParserunderSAFE_FOR_XML, and DOMPurify's attribute rule removes the wholedata-trix-attachmentattribute on a match, before theforceKeepAttrset by Trix'suponSanitizeAttributehook is honored. Quoted mail with an embedded<style>block is the common case: in HEY, 67 of 153 real mail bodies lost their embedded content on paste. Dragging the same content was lossless.What #1337 does, and what this adds
#1337 stashes every
data-trix-*value inuponSanitizeAttributeand puts it back inafterSanitizeAttributesonce DOMPurify has dropped it. That stops the loss at the sanitizer for everydata-trix-*attribute, without changing the HTML Trix emits.This PR removes the trigger from the value itself, so nothing has to be put back:
AttachmentViewwritesdata-trix-attachmentanddata-trix-attributeswith<and>escaped as\u003c/\u003e. Trix's own HTML then never carries aSAFE_FOR_XMLtrigger, so it survives a paste into any Trix build — including releases without either fix — and into anything else that runs DOMPurify in that mode.HTMLSanitizerrewrites those two attributes the same way before DOMPurify runs, so stored, server-rendered and older-Trix HTML with literal brackets is safe by construction: the rule has nothing to match.The rewrite is lossless: in JSON text, angle brackets only occur inside string literals, where the escapes spell the same characters, so
JSON.parsereads back the same value. Only values that already parse as JSON are rewritten; a malformed value is left alone, sinceHTMLParserignores it either way.With the two JSON attributes escaped before DOMPurify runs, #1337's restore hook had nothing left to restore except malformed or non-JSON
data-trix-*values thatSAFE_FOR_XMLdeliberately dropped, so this branch removes it (0004e08) and relies on escaping plusforceKeepAttr. A non-JSONdata-trix-*value carrying a trigger is now removed, as DOMPurify intends, and a test pins that.The
</html>truncationStored-shape attachment JSON with literal brackets exposed a second, older bug on the same path.
HTMLSanitizertrims pasted HTML at</html>because Windows browsers can append clipboard bytes after it (778f4d8, 2016), which the parser would otherwise append to the body as text. The trim was a plain string replace, so a</html>inside an attribute value — an attachment whose content is a full HTML document, which is what a quoted mail is — cut the string mid-attribute and dropped the attachment and everything after it. The trim now asks the browser's tokenizer which</html>is the tag: every</htmlin the string is swapped for a marker start tag and the string parsed once, and the first marker that comes out as an element was tokenized as a tag, while one inside an attribute value, a comment or a raw-text element such as<style>stays text. The extra parse runs only when the string contains</htmlat all. Dropping the trim outright was checked and rejected: the fragment parser does reparent post-</html>text into the body, so the Word-paste test regresses without it.What the rebase changed
process_timeout) are byte-identical to the ones in Pasting an embedded-content attachment may destroy it #1337 and dropped out of the rebase; Pasting an embedded-content attachment may destroy it #1337 is green with them, so there is nothing to split into a separate PR.keeps Trix attributes containing markup when sanitizing for XMLnow asserts parse-equality plus no raw angle brackets instead of byte-identity, because the value comes back escaped under this layer. Its other tests (html_parser_test,pasting_test) pass unchanged.html_sanitizer.jsand its test describe the composition rather than each fix on its own.Proof
helpers/strings_test: escapes every bracket, round-trips throughJSON.parse(backslashes before brackets, pre-escaped input, surrogate pairs, nesting), idempotent.html_sanitizer_test: attachment and caption JSON containing each of the twelve sequences in DOMPurify's regex survive underSAFE_FOR_XMLwith no raw brackets; a</style>attachment nested inside another attachment's content survives; malformed JSON is untouched; a non-JSONdata-trix-*value carrying-->or</style>is removed underSAFE_FOR_XML; clipboard bytes after</html>are cut while a</html>inside an attribute value, a comment closed with--!>, a<textarea>or a<style>is not; two</html>inside one attribute value, an input-supplied marker element, and</html\u00a0>are each handled as the browser tokenizes them.html_parser_test: the pasted markup parses back to an attachment with the original content and caption, including the nested case, and an attachment whose content is a full<html>…</html>document keeps both the attachment and the paragraph after it.document_view_test: rendered attachment JSON carries no raw brackets and parses back to the original.pasting_test(system): pasting the editor's ownvaluefor a<style>-bearing attachment with a</style>caption yields a second, identical attachment; the same for stored-shape markup with literal brackets whose content closes both</style>and</html>.attachment_view.jsalone fails the render tests, revertinghtml_sanitizer.jsalone fails the sanitizer and parser tests.